-
Notifications
You must be signed in to change notification settings - Fork 424
Rename variables if the name matches class name #5454
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Rename variables if the name matches class name #5454
Conversation
b83c9b8
to
071fb76
Compare
071fb76
to
8551095
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Neat addition, thanks! Couple small requests, but otherwise looks good.
rewrite-core/src/main/java/org/openrewrite/internal/StringUtils.java
Outdated
Show resolved
Hide resolved
public A2 method(A2 a2) { | ||
return a2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Neat idea indeed to rename these as well!
rewrite-java/src/main/java/org/openrewrite/java/ChangeType.java
Outdated
Show resolved
Hide resolved
rewrite-core/src/main/java/org/openrewrite/internal/StringUtils.java
Outdated
Show resolved
Hide resolved
rewrite-java/src/main/java/org/openrewrite/java/ChangeType.java
Outdated
Show resolved
Hide resolved
rewrite-java-test/src/test/java/org/openrewrite/java/ChangeTypeTest.java
Outdated
Show resolved
Hide resolved
Hey @timtebeek, could you ping rest of the team to take a look? 😉 |
Got back over the weekend, thanks! Having a look now. Did a quick extra check that we only touch variables where the type changed: @Test
void changeMethodVariableName() {
rewriteRun(
spec -> spec.recipe(new ChangeType("a.A1", "a.A2", true)),
java(
"""
package a;
public class A1 {
}
"""
),
java(
"""
package a;
public class A2 {
}
"""
),
java(
"""
package org.foo;
import a.A1;
import a.A2;
public class Example {
public A1 method1(A1 a1) {
return a1;
}
public A2 method2(A2 a1) {
return a1; // Unchanged
}
}
""",
"""
package org.foo;
import a.A2;
public class Example {
public A2 method1(A2 a2) {
return a2;
}
public A2 method2(A2 a1) {
return a1; // Unchanged
}
}
"""
)
);
} |
Hi @timtebeek, did you take a look? I would love to proceed with this PR 😉 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks both; took a little longer to figure out what seemed "off" at the earlier implementation. Figured it out just now and applied changes to make better use of RenameVariable
and keep all changes inside visitVariable
. Previously only the printed name of the variable was changed, but the underlying type informations was not updated from the same point. That might then lead to problems down the line. With the changes made just now I'm more confident to merge this, as it's such a frequently used recipe. Thanks again for the suggestion, tests and initial implementation!
Hi @timtebeek, thanks for the changes! The only thing I don't understand is the commit Do not expect Kotlin changes just yet. Does it mean this recipe won't rename variables in Kotlin? That would be bad news for my company, which have most of the code written in Kotlin 😞 Or maybe the "just yet" means that more commits will follow? 😛 |
I've not yet worked out why we're not seeing change for Kotlin, and my IDE was throwing up some unrelated issues in trying to figure it out. It might be that |
Fixed in 3b1f9d1; thanks ! |
What's your motivation?
When I rename a type in IntelliJ IDEA (for example
SomeType
toAnotherType
) it will automatically rename variables with the same name as the class, but starting with lowercase character (sosomeType
toanotherType
).This PR introduces the above feature to
org.openrewrite.java.ChangeType
recipe.Checklist